Fixed UDP sendto if IP version not match #10834

tymoteuszblochmobica · 2019-06-14T09:25:32Z

Description

Fixed LWIPStack class socket_sendto member to fail if interface IP4/6 version differ from destination adress IP version

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@SeppoTakalo
@kivaisan

Release Notes

SeppoTakalo · 2019-06-14T09:30:19Z

This looks good. You managed to find the solution without touching the LwIP internals.

0xc0170 · 2019-06-14T09:32:06Z

features/lwipstack/LWIPStack.cpp


 nsapi_addr_t addr = address.get_addr();
 if (!convert_mbed_addr_to_lwip(&ip_addr, &addr)) {
 return NSAPI_ERROR_PARAMETER;
 }

+ convert_lwip_addr_to_mbed(&interface_addr, get_ip_addr(true, &default_interface->netif));
+ if(addr.version != interface_addr.version){


astyle failure fix needed here

kivaisan · 2019-06-14T09:37:25Z

features/lwipstack/LWIPStack.cpp


 nsapi_addr_t addr = address.get_addr();
 if (!convert_mbed_addr_to_lwip(&ip_addr, &addr)) {
 return NSAPI_ERROR_PARAMETER;
 }

+ convert_lwip_addr_to_mbed(&interface_addr, get_ip_addr(true, &default_interface->netif));
+ if(addr.version != interface_addr.version){


Does this work in dual stack case when you have both addresses? get_ip_addr will return the preferred address (defined with lwip.ip-ver-pref configuration), but the application uses the other address version?

If there is both IP4 and IP6 on netif then IP_VERSION_PREF must be properly set.

In order to get rid of prefered dependency, how about something like this:

lwip_tools.cpp:

bool LWIP::dualstack_enabled() { #if LWIP_IPV4 && LWIP_IPV6 if (get_ipv4_addr(netif) && get_ipv6_addr(netif)) { return true; } #endif return false; }

and then in version check first check that dualstack is not on:
if (!dualstack_enabled() && addr.version != interface_addr.version) {

kjbracey · 2019-06-14T10:47:42Z

features/lwipstack/LWIPStack.cpp


 nsapi_addr_t addr = address.get_addr();
 if (!convert_mbed_addr_to_lwip(&ip_addr, &addr)) {
 return NSAPI_ERROR_PARAMETER;
 }

+ convert_lwip_addr_to_mbed(&interface_addr, get_ip_addr(true, &default_interface->netif));
+ if (addr.version != interface_addr.version) {


Dual-mode is possible - that fails sending to IPv4 if you're dual IPv4 and IPv6 with IPv6 preferred.

Not to mention the issue of multiple potential interfaces - that blocks IPv6 on the IPv6 interface if your default is IPv4.

To get a good answer here, I think you'd have to get the error from lwIP. Seems like lwIP is probably generating an error internally, but it gets lost. netconn_send doesn't extract the err value from the msg that netconn_do_send fills in?

Fixed dualstack issue as Kimmo suggested.
Checked that if both ip4/6 enabled netconn returns ERR_OK because ip_route always finds the netif.

adbridge · 2019-06-15T09:50:41Z

@kjbracey-arm please review updates

kjbracey

Restating the logic here, to make sure I've got it right:

"If the default interface doesn't have both IPv4 and IPv6 addresses, check to see if this address matches the type of the preferred (and hence only) IP address of the default interface."

Still a bit nervous that this is fragile. What if your default interface is IPv4 only, but you've got a IPv6 interface?

Maybe with default routing that setup wouldn't work anyway - traffic always goes to the default, but if you've done a NSAPI_BIND_TO_DEVICE, you do need to look at the bound interface, right, not the default one?

tymoteuszblochmobica · 2019-06-17T11:07:53Z

Kevin, your proposal has sense. However it is a kind of core LWIP functionality improvement.
This change probably needs discusion. Maybe the best is to finalize PR at current state and open new ticket to improve this.
What do you think about it?
FYI @mikaleppanen, @kivaisan, @SeppoTakalo

kjbracey · 2019-06-17T13:19:31Z

My main concern is that this can break multihoming, by not looking at the interface the socket is bound to. Not sure you need to necessarily change lwIP core for that - if you can record the bound interface in the glue layers, that might be sufficient. I believe more complex multi-interface routing is not supported, but locking sockets to interfaces is supposed to work.

If you're not currently testing split IPv4/IPv6 multihoming, I guess you get away with it for now, but I'd want an immediate issue for that limitation created if you merge this.

tymoteuszblochmobica · 2019-06-17T16:55:38Z

Currently if the socket is bound to interface, ip_route uses that netif instead of default one according to multihoming. But socket is bound just to interface name regardless of ip version. NSAPI_BIND_TO_DEVICE can bind netif v4 or v6 according to the user requirements.
Default netif is chosen only if previous attempts fails.

In case of dual stack and each netif has only one ip version there is no issue with preferred IP.

Im using "get_ip_addr" with "true" parameter so if preffered ip not exist it returns not preffered.
Thus if netif bound to socket has ip4 and preff ip is v6 it will return ip4 because ip6 not exist.
Similarly if socket has ip6 only and preff ip is v4 it return ip6.

The problem exist if dual stack is enabled and one netif has both ip4 and ip6. This case wasn't tested during multihoming launch.
Therefore little improvement is needed to ensure proper handling this case.
Is Jira ticket enough or should i create Github issue also?

kjbracey · 2019-06-18T07:38:44Z

I think the correct version is relatively simple - arguably simpler than this PR.

Detail needs to be filled in, but structurally:

struct netif *netif = netif_get_by_index(s->conn->ip.netif_idx);
if (!netif) {
     netif = &default_interface->netif;
}
if (netif) {
    if ((addr.version == IPV4 && !get_ipv4_addr(netif) ||
        (addr.version == IPV6 && !get_ipv6_addr(netif)) {
          return ERR_WHATEVER;
    }
 }

This is relatively cautious - only blocking the call when it has a good idea what's going on. If it doesn't recognise the address version or know what the default interface is it will proceed. Maybe that could be tightened, depending on what you want.

Note that as netif_idx is in the common IP_PCB, you can just read it directly without worrying about socket type. lwip_netconn_do_bind_if could have been simplified like this - you don't need separate raw_bind_netif etc.

tymoteuszblochmobica · 2019-06-18T13:34:25Z

Applied

kjbracey · 2019-06-18T13:56:52Z

features/lwipstack/LWIPStack.cpp

+ netif_ = &default_interface->netif;
+ }
+ if (netif_) {
+ if ((addr.version == NSAPI_IPv4 && !get_ipv4_addr(netif_)) ||


BTW, I didn't consider V4/V6-only compilation - it looks like these get_ipvx_addr aren't defined if LWIP_IPVX isn't enabled. You should probably fiddle them to have them defined and return constant NULL in that case - will be neater than adding ifdefs here.

Changed get_ipvx_addr LWIP_IPV4/6

…on differ from destination adress IP version

adbridge · 2019-06-20T15:41:50Z

@kjbracey-arm could you please re-review after the recent change?

0xc0170 · 2019-06-24T08:23:08Z

CI started

mbed-ci · 2019-06-24T09:31:51Z

Test run: FAILED

Summary: 6 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_mbed2-build-IAR
jenkins-ci/mbed-os-ci_mbed2-build-ARM
jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-IAR
jenkins-ci/mbed-os-ci_build-GCC_ARM

0xc0170 · 2019-06-24T09:32:17Z

Aborted the job unfortunately as we have to reschedule one possibly rc4 PR.
We will restart this one once it is in.

0xc0170 · 2019-06-24T21:33:18Z

Ci restarted

mbed-ci · 2019-06-25T04:12:48Z

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

SeppoTakalo approved these changes Jun 14, 2019

View reviewed changes

0xc0170 reviewed Jun 14, 2019

View reviewed changes

kivaisan reviewed Jun 14, 2019

View reviewed changes

tymoteuszblochmobica force-pushed the udp branch from 6fd059e to 5e1cc17 Compare June 14, 2019 09:51

0xc0170 approved these changes Jun 14, 2019

View reviewed changes

kivaisan approved these changes Jun 14, 2019

View reviewed changes

0xc0170 added needs: CI release-version: 5.13.1 labels Jun 14, 2019

kjbracey reviewed Jun 14, 2019

View reviewed changes

tymoteuszblochmobica force-pushed the udp branch from 5e1cc17 to 82ed294 Compare June 14, 2019 15:59

adbridge added needs: review and removed needs: CI labels Jun 15, 2019

kjbracey reviewed Jun 17, 2019

View reviewed changes

tymoteuszblochmobica force-pushed the udp branch 3 times, most recently from 568ae20 to f8cd118 Compare June 18, 2019 13:17

kjbracey approved these changes Jun 18, 2019

View reviewed changes

kjbracey reviewed Jun 18, 2019

View reviewed changes

Fixed LWIPStack socket_sendto member to fail if interface IP4/6 versi…

b272c5f

…on differ from destination adress IP version

tymoteuszblochmobica force-pushed the udp branch from f8cd118 to b272c5f Compare June 19, 2019 12:16

kjbracey approved these changes Jun 24, 2019

View reviewed changes

0xc0170 removed the needs: review label Jun 24, 2019

0xc0170 added the needs: CI label Jun 24, 2019

0xc0170 added ready for merge and removed needs: CI labels Jun 25, 2019

0xc0170 merged commit 4b438ac into ARMmbed:master Jun 25, 2019

0xc0170 removed the ready for merge label Jun 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed UDP sendto if IP version not match #10834

Fixed UDP sendto if IP version not match #10834

tymoteuszblochmobica commented Jun 14, 2019

SeppoTakalo commented Jun 14, 2019

0xc0170 Jun 14, 2019

tymoteuszblochmobica Jun 14, 2019

kivaisan Jun 14, 2019

tymoteuszblochmobica Jun 14, 2019 •

edited

Loading

kivaisan Jun 14, 2019 •

edited

Loading

tymoteuszblochmobica Jun 14, 2019

kjbracey Jun 14, 2019

tymoteuszblochmobica Jun 14, 2019

adbridge commented Jun 15, 2019

kjbracey left a comment

tymoteuszblochmobica commented Jun 17, 2019 •

edited

Loading

kjbracey commented Jun 17, 2019

tymoteuszblochmobica commented Jun 17, 2019 •

edited

Loading

kjbracey commented Jun 18, 2019 •

edited

Loading

tymoteuszblochmobica commented Jun 18, 2019

kjbracey Jun 18, 2019

tymoteuszblochmobica Jun 19, 2019

adbridge commented Jun 20, 2019

0xc0170 commented Jun 24, 2019

mbed-ci commented Jun 24, 2019

0xc0170 commented Jun 24, 2019

0xc0170 commented Jun 24, 2019

mbed-ci commented Jun 25, 2019

Fixed UDP sendto if IP version not match #10834

Fixed UDP sendto if IP version not match #10834

Conversation

tymoteuszblochmobica commented Jun 14, 2019

Description

Pull request type

Reviewers

Release Notes

SeppoTakalo commented Jun 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tymoteuszblochmobica Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

kivaisan Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adbridge commented Jun 15, 2019

kjbracey left a comment

Choose a reason for hiding this comment

tymoteuszblochmobica commented Jun 17, 2019 • edited Loading

kjbracey commented Jun 17, 2019

tymoteuszblochmobica commented Jun 17, 2019 • edited Loading

kjbracey commented Jun 18, 2019 • edited Loading

tymoteuszblochmobica commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adbridge commented Jun 20, 2019

0xc0170 commented Jun 24, 2019

mbed-ci commented Jun 24, 2019

0xc0170 commented Jun 24, 2019

0xc0170 commented Jun 24, 2019

mbed-ci commented Jun 25, 2019

tymoteuszblochmobica Jun 14, 2019 •

edited

Loading

kivaisan Jun 14, 2019 •

edited

Loading

tymoteuszblochmobica commented Jun 17, 2019 •

edited

Loading

tymoteuszblochmobica commented Jun 17, 2019 •

edited

Loading

kjbracey commented Jun 18, 2019 •

edited

Loading